packages/jumpstarter/jumpstarter/common/grpc.py (1)

36-36: Consider the fragility of accessing private SSL attributes.

The code accesses ._sslobj, which is a private attribute of Python's SSL implementation. While this is sometimes necessary for low-level certificate extraction, it could break in future Python versions. Consider adding a comment documenting this dependency and the Python version compatibility requirements.

Example documentation:

     # Extract certificates
+    # Note: Accessing _sslobj is implementation-dependent and tested with Python 3.x
+    # See: https://github.com/python/cpython/blob/main/Lib/ssl.py
     cert_chain = writer.get_extra_info("ssl_object")._sslobj.get_unverified_chain()

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/jumpstarter/jumpstarter/common/grpc.py (1)

37-45: Make cert-chain extraction robust across Python versions (bytes vs str, DER vs PEM).

Current concatenation assumes cert.public_bytes() returns str; on other runtimes it returns bytes or get_unverified_chain() yields DER bytes, causing crashes. Handle all cases and always return PEM bytes.

Apply this diff:

-        cert_chain = writer.get_extra_info("ssl_object")._sslobj.get_unverified_chain()
-        root_certificates = ""
-        for cert in cert_chain:
-            root_certificates += cert.public_bytes()
+        cert_chain = writer.get_extra_info("ssl_object")._sslobj.get_unverified_chain()
+        root_certs_bytes = bytearray()
+        for cert in cert_chain:
+            # CPython <3.13: items may be _ssl.Certificate with .public_bytes() (str or bytes)
+            # CPython >=3.13: items may be DER bytes
+            if hasattr(cert, "public_bytes"):
+                pem = cert.public_bytes()
+                if isinstance(pem, str):
+                    pem = pem.encode()
+                root_certs_bytes.extend(pem)
+            else:
+                # DER -> PEM string, then to bytes
+                if isinstance(cert, str):
+                    cert = cert.encode()
+                pem_str = ssl.DER_cert_to_PEM_cert(cert)
+                root_certs_bytes.extend(pem_str.encode())
         logger.debug(f"Successfully extracted {len(cert_chain)} certificate(s) from {ip_address}:{port}")
-
-        return root_certificates.encode()
+        return bytes(root_certs_bytes)
     finally:
         writer.close()

Does Python 3.13’s ssl.SSLSocket.get_unverified_chain() return a list of DER-encoded bytes rather than certificate objects?

Note: optionally await writer.wait_closed() to drain TLS close; you previously opted not to.

packages/jumpstarter/jumpstarter/common/grpc.py (2)

72-76: De-duplicate resolved IPs before spawning tasks.

Avoid redundant connections when getaddrinfo returns duplicates (A/AAAA, multiple records).

Apply this diff:

-            # Log resolved IPs
-            resolved_ips = [sockaddr[0] for _, _, _, _, sockaddr in addr_info]
+            # De-duplicate by IP
+            unique_by_ip = {}
+            for item in addr_info:
+                ip = item[4][0]
+                unique_by_ip.setdefault(ip, item)
+            addr_info = list(unique_by_ip.values())
+            # Log resolved IPs
+            resolved_ips = [sockaddr[0] for _, _, _, _, sockaddr in addr_info]
             logger.debug(
-                f"Resolved {parsed.hostname} to {len(resolved_ips)} IP(s): {', '.join(resolved_ips)}"
+                f"Resolved {parsed.hostname} to {len(resolved_ips)} unique IP(s): {', '.join(resolved_ips)}"
             )

---

121-124: Cancel and drain remaining tasks to avoid “Task exception was never retrieved” warnings.

After cancellation, await the tasks with return_exceptions=True.

Apply this diff:

             finally:
                 # Cancel any remaining tasks
-                for task in tasks:
-                    if not task.done():
-                        task.cancel()
+                pending = [t for t in tasks if not t.done()]
+                for t in pending:
+                    t.cancel()
+                if pending:
+                    await asyncio.gather(*pending, return_exceptions=True)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/jumpstarter/jumpstarter/common/grpc.py (4)

120-124: Await cancelled tasks to avoid “Task was destroyed but it is pending!” warnings.

After task.cancel(), await them with gather(..., return_exceptions=True) to let cancellations settle.

             finally:
                 # Cancel any remaining tasks
                 for task in tasks:
                     if not task.done():
                         task.cancel()
+                # Ensure cancelled tasks are awaited
+                await asyncio.gather(*(t for t in tasks if not t.done()), return_exceptions=True)

---

73-76: Deduplicate resolved IPs to avoid redundant connection attempts.

getaddrinfo may return duplicate addresses (IPv4/IPv6 or multiple rows per IP). Dedup to reduce load.

-            resolved_ips = [sockaddr[0] for _, _, _, _, sockaddr in addr_info]
+            resolved_ips = []
+            for _, _, _, _, sockaddr in addr_info:
+                ip = sockaddr[0]
+                if ip not in resolved_ips:
+                    resolved_ips.append(ip)
@@
-            for _family, _type, _proto, _canonname, sockaddr in addr_info:
-                ip_address = sockaddr[0]
-                task = asyncio.create_task(try_with_ip(ip_address))
+            for ip_address in resolved_ips:
+                task = asyncio.create_task(try_with_ip(ip_address))
                 tasks.append(task)

Also applies to: 91-95

---

43-45: Optional: also await writer.wait_closed() for graceful close.

Non-blocking close() is OK, but awaiting closure improves hygiene under high churn.

     finally:
-        writer.close()
+        writer.close()
+        try:
+            await writer.wait_closed()
+        except Exception:
+            pass  # best-effort

---

131-134: Broaden env var truthiness for insecure mode.

Accept common true values to ease ops usage.

-    if tls_config.insecure or os.getenv("JUMPSTARTER_GRPC_INSECURE") == "1":
+    insecure_env = os.getenv("JUMPSTARTER_GRPC_INSECURE", "").strip().lower()
+    if tls_config.insecure or insecure_env in {"1", "true", "yes", "on"}:
         return await _ssl_channel_credentials_insecure(target, timeout)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro